Handle new execute_intent non-raising behaviour - #1048
Conversation
a426727 to
60406e6
Compare
Edouard-chin
left a comment
Edouard-chin
left a comment
There was a problem hiding this comment.
It would be good to also rm -rf the Gemfile.lock before the bundle install steps kicks in on CI. This way we are sure bundler picks the latest Rails edge
…red on the query intent and raise it to trip the circuit breaker, then rescue and allow Rails to continue execution.
0832de9 to
e2f1cae
Compare
764c346 to
cea4e92
Compare
|
Alright, I took a crack at improving the testing situation for this gem. Now the main gemfile and all the testing gemfiles lock to the latest released version, and I added I had to keep minitest locked to 5.6. If we bump it to 6.0 we get some test failures, so that can be done in a separate PR. Also, this uncovered we weren't handling |
Edouard-chin
left a comment
Edouard-chin
left a comment
There was a problem hiding this comment.
@adrianna-chang-shopify Usually committing the main Gemfile.lock is enough.
The lockfile in gemfiles/ are mostly CI only, and it's rare that developers need to run BUNDLE_GEMFILE=gemfiles/Rails_8.2 bundle exec rake test. In the event that they have a failing CI for a specific gemfile, then it's better to not have a lockfile committed so that tests run with the same dependencies as on CI (the latest one).
Committing the Gemfile.lock remove all possible friction to contribute to a project
@Edouard-chin , yep fair, in this case though, should we remove all lockfiles under We can probably also drop the |
@adrianna-chang-shopify Ah yes, great point. I think we should |
cea4e92 to
2942b8e
Compare
Consistently run all tests against both the latest released Rails version as well as Rails edge. Also drop tests for Ruby 3.2 as it is EOL.
cc06ea3 to
47cba91
Compare
…s/rails@e61b5e24 rescues these and converts them to ActiveRecord::ConnectionNotEstablished, but that is only on edge. Now that we are also testing on 8.1 it exposed that we are not handling the 8.1 behaviour which is to raise a PG::ConnectionBad.
47cba91 to
e4c7e51
Compare
3 participants
rails/rails#58049 refactored
execute_intentto no longer raise errors. Instead execution errors are stored on the query intent and can be checked later by the caller. This is to pave the way for better async execution.However, semian relies on the raising behaviour to trip the circuit breaker. With this change to Rails, any query that fails mid execution will be considered successful by Semian. And indeed
ActiveRecordTrilogyAdapterTest#test_read_timeout_error_opens_the_circuitfails when we bump Rails to this commit.To solve it, in our
execute_intentpatch we can check the intent for an error and raise it within theacquire_semian_resourceblock to trip the circuit breaker. Then we can rescue it and return the result to match the new Rails behaviour.